feat(api): encrypt async job content artifacts#291
Conversation
Encrypt async final report output and selected artifact event content at rest, including static-key and Vault Transit modes, health/readiness checks, report read decryption, and coverage for worker, route, and event-store behavior. Signed-off-by: Tanner Leach <tleach@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds application-level content encryption ( ChangesAsync Job Content Encryption
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant JobsRoute
participant Runner
participant ContentEncryptionManager
participant EventStore
Client->>JobsRoute: POST /v1/jobs/async/submit
JobsRoute->>ContentEncryptionManager: require_content_encryption_ready_for_submission_async()
ContentEncryptionManager-->>JobsRoute: ready / unavailable / config error
JobsRoute->>Runner: run_agent_job(content_encryption_policy)
Runner->>ContentEncryptionManager: create_job_content_cipher(job_id)
ContentEncryptionManager-->>Runner: JobContentCipher
Runner->>EventStore: EventStore(content_cipher)
Runner->>ContentEncryptionManager: update_job_output(encrypted report)
Client->>JobsRoute: GET /v1/jobs/async/job/{id}/report
JobsRoute->>ContentEncryptionManager: read_job_output_async(stored_output)
ContentEncryptionManager-->>JobsRoute: decrypted report / 503 / 500
JobsRoute-->>Client: report response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Document the default-off behavior, exact encrypted field scope, Vault concurrency and retry model, forward-only rollout constraints, stable key identity requirement, and database-writer replay limitation. Signed-off-by: Tanner Leach <tleach@nvidia.com>
Protect shared encryption state under concurrency, offload blocking Vault work from async routes, surface encrypted event failures, and harden Vault readiness and retry behavior. Add regression coverage for concurrency, async responsiveness, readiness, retry classification, explicit API and SSE failures, and default-off configuration. Signed-off-by: Tanner Leach <tleach@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
458-462: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate the submit route response contract for encryption failures.
The handler now returns
503for encryption unready and500for invalid encryption config, but OpenAPI still documents503only as Dask unavailable and omits500. As per path instructions, “Treat API, auth, and job-runner changes as externally visible contracts.”Proposed fix
responses={ - 503: {"description": "Dask scheduler not available"}, + 500: {"description": "Content encryption configuration is invalid"}, + 503: {"description": "Dask scheduler not available or content encryption is not ready"}, },Also applies to: 477-533
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/aiq_api/src/aiq_api/routes/jobs.py` around lines 458 - 462, The submit route response contract in jobs.py is out of sync with the handler’s encryption error paths: it documents only 503 for Dask availability and does not include the new 500 invalid-encryption-config case. Update the OpenAPI responses for the submit route handler (the route around the submit endpoint, including the shared responses used across the related blocks) so encryption-unready is documented as 503 and invalid encryption config is documented as 500, while keeping the existing 400 and 422 entries intact.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontends/aiq_api/src/aiq_api/jobs/crypto.py`:
- Around line 25-33: Reject non-finite numeric environment values in the numeric
parsing/validation path used by the crypto job settings, since float("nan") and
float("inf") currently slip through. Update the validator/helper that reads
these environment values so it explicitly requires finite numbers before
accepting TTLs/timeouts, and apply the same check anywhere the shared parsing
logic is reused in the referenced sections. Use the existing env-setting symbols
in this module to locate the validator and keep the rejection behavior
consistent across readiness TTL, cache TTL, and Vault timeout inputs.
- Around line 904-912: In decode_envelope, malformed non-ASCII envelope payloads
are not being mapped to the encrypted-data error because _b64url_decode() can
raise UnicodeEncodeError. Update the exception handling in decode_envelope to
catch UnicodeEncodeError alongside the existing decode/JSON/value errors and
re-raise ContentEncryptionInvalidData, so all malformed aiqenc envelopes are
classified consistently.
- Around line 99-147: ContentEncryptionConfig is exposing secret-bearing fields
in both dataclass repr and the signature tuple. Update the
ContentEncryptionConfig dataclass to mark static_key, vault_role_id, and
vault_secret_id as repr=False, and change the signature property to use a
non-reversible fingerprint or placeholder derived from those values instead of
including the raw credentials directly. Keep the rest of the signature logic in
ContentEncryptionConfig unchanged.
In `@frontends/aiq_api/tests/test_content_encryption_routes.py`:
- Around line 101-102: The submit tests are patching the wrong symbol, so they
never intercept the route’s real submission path. Update the test setup in
test_content_encryption_routes to patch the hook used by register_job_routes,
specifically submit_authorized_job as resolved in aiq_api.routes.jobs, rather
than aiq_api.jobs.submit.submit_agent_job. This will ensure the 503 cases
actually verify that submission was skipped.
In `@tests/aiq_agent/jobs/test_runner.py`:
- Around line 600-671: This test is using a shared SQLite database URL, which
can leak state and cause flakiness. Update the
`test_final_output_encryption_failure_marks_failure_without_plaintext_write`
setup in `test_runner.py` to build the `sqlite:///...` job store URL from a
per-test temporary path (use the test’s `tmp_path` fixture) instead of
`sqlite:///./test.db`, keeping the rest of the `run_agent_job` invocation
unchanged.
---
Outside diff comments:
In `@frontends/aiq_api/src/aiq_api/routes/jobs.py`:
- Around line 458-462: The submit route response contract in jobs.py is out of
sync with the handler’s encryption error paths: it documents only 503 for Dask
availability and does not include the new 500 invalid-encryption-config case.
Update the OpenAPI responses for the submit route handler (the route around the
submit endpoint, including the shared responses used across the related blocks)
so encryption-unready is documented as 503 and invalid encryption config is
documented as 500, while keeping the existing 400 and 422 entries intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b7e57310-0a09-4e09-afe5-7d2b4bc579b2
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
deploy/Dockerfiledocs/source/deployment/content-encryption.mddocs/source/deployment/index.mddocs/source/index.mdfrontends/aiq_api/pyproject.tomlfrontends/aiq_api/src/aiq_api/jobs/crypto.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/tests/test_job_access.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Run Harbor skill eval
⚠️ CI failures not shown inline (2)
GitHub Actions: AIQ CI / 3_Script Validation.txt: fix(api): harden async job content encryption
Conclusion: failure
Current runner version: '2.335.1'
##[group]Runner Image Provisioner
Hosted Compute Agent
Version: 20260611.554
Commit: 5e0782fdc9014723d3be820dd114dd31555c2bd1
Build Date:
Worker ID: {0841f878-952d-47ee-86aa-3d5642c7e518}
Azure Region: westcentralus
##[endgroup]
##[group]Operating System
Ubuntu
24.04.4
LTS
##[endgroup]
##[group]Runner Image
Image: ubuntu-24.04
Version: 20260622.220.1
Included Software: https://github.com/actions/runner-images/blob/ubuntu24/20260622.220/images/ubuntu/Ubuntu2404-Readme.md
Image Release: https://github.com/actions/runner-images/releases/tag/ubuntu24%2F20260622.220
##[endgroup]
##[group]GITHUB_TOKEN Permissions
Contents: read
Metadata: read
##[endgroup]
Secret source: Actions
Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v4' (SHA:34e114876b0b11c390a56381ad16ebd13914f8d5)
Download action repository 'actions/setup-python@v5' (SHA:a26af69be951a213d495a4c3e4e4022e16d87065)
Download action repository 'astral-sh/setup-uv@v4' (SHA:38f3f104447c67c051c4a08e39b64a148898af3a)
Complete job name: Script Validation
Node 20 is being deprecated. This workflow is running with Node 24 by default. If you need to temporarily use Node 20, you can set the ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION=true environment variable. For more information see: https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/
##[group]Run actions/checkout@v4
with:
repository: NVIDIA-AI-Blueprints/aiq
***REDACTED***
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
fetch-depth: 1
fetch-tags: false
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
##[endgroup]
Syncing repository: NVIDIA-AI-Blueprints/aiq
##[group]Getting Git version info
Working directory is '/home/runner/work/aiq/aiq'
[command]/usr/bin/git version...
GitHub Actions: AIQ CI / Script Validation: fix(api): harden async job content encryption
Conclusion: failure
##[group]Run . .venv/bin/activate
�[36;1m. .venv/bin/activate�[0m
�[36;1mchmod +x ci/scripts/test_scripts.sh�[0m
�[36;1mci/scripts/test_scripts.sh --skip-setup�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.13.14/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.13.14/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.13.14/x64/lib
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
================================================
AI-Q Blueprint - Script Tests
================================================
Repository: /home/runner/work/aiq/aiq
Scripts: /home/runner/work/aiq/aiq/scripts
============================================
Testing Bash Syntax
============================================
�[0;32m✅ PASS�[0m: dev.sh - valid bash syntax
�[0;32m✅ PASS�[0m: setup.sh - valid bash syntax
�[0;32m✅ PASS�[0m: start_as_skill.sh - valid bash syntax
�[0;32m✅ PASS�[0m: start_cli.sh - valid bash syntax
�[0;32m✅ PASS�[0m: start_e2e.sh - valid bash syntax
�[0;32m✅ PASS�[0m: start_server_in_debug_mode.sh - valid bash syntax
�[1;33m⏭️ SKIP�[0m: setup.sh - skipped (--skip-setup flag)
============================================
Testing --help Flags
============================================
�[0;32m✅ PASS�[0m: start_cli.sh --help
�[0;32m✅ PASS�[0m: start_server_in_debug_mode.sh --help
============================================
Testing Virtual Environment Checks
============================================
�[0;32m✅ PASS�[0m: start_cli.sh - venv check works
�[0;32m✅ PASS�[0m: start_server_in_debug_mode.sh - venv check works
============================================
Testing Pytest Integration
============================================
tests/aiq_agent/agents/chat_resear...
🧰 Additional context used
📓 Path-based instructions (7)
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update the docs under docs/source/ when behavior, configuration, or workflows change
Files:
docs/source/index.mddocs/source/deployment/index.mddocs/source/deployment/content-encryption.md
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
docs/source/index.mdfrontends/aiq_api/pyproject.tomldeploy/Dockerfiledocs/source/deployment/index.mdfrontends/aiq_api/tests/test_job_access.pyfrontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pydocs/source/deployment/content-encryption.mdfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/source/index.mddocs/source/deployment/index.mddocs/source/deployment/content-encryption.md
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
deploy/Dockerfile
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
frontends/aiq_api/tests/test_job_access.pyfrontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
frontends/aiq_api/tests/test_job_access.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.py
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}: Treat API, auth, and job-runner changes as externally visible contracts. Check authorization boundaries,
request tracing, async job lifecycle, websocket reconnect behavior, error responses, and cross-user data isolation.
Require tests for route behavior, access decisions, and job state transitions when those surfaces change.
Files:
frontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
🧠 Learnings (1)
📚 Learning: 2026-06-14T17:49:00.640Z
Learnt from: torkian
Repo: NVIDIA-AI-Blueprints/aiq PR: 273
File: frontends/aiq_api/tests/test_sse_reconnect_cursor.py:384-401
Timestamp: 2026-06-14T17:49:00.640Z
Learning: When using `unittest.mock.patch` for code that imports dependencies inside functions/generators (e.g., inside `aiq_api.routes.jobs`), don’t patch via an attribute that doesn’t exist on the consuming module. If the generator does `from ..jobs.event_store import EventStore` inside the generator body, then `aiq_api.routes.jobs` will not have an `EventStore` attribute; patch the source class/method in its defining module instead (e.g., `aiq_api.jobs.event_store.EventStore.get_events_async`). Patching `aiq_api.routes.jobs.EventStore...` would raise `AttributeError` because that symbol is not present at module scope.
Applied to files:
frontends/aiq_api/tests/test_job_access.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.py
🪛 ast-grep (0.44.0)
frontends/aiq_api/tests/test_event_store_content_encryption.py
[info] 114-114: use jsonify instead of json.dumps for JSON output
Context: json.dumps(raw_event)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 140-140: use jsonify instead of json.dumps for JSON output
Context: json.dumps(raw_event)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
frontends/aiq_api/tests/test_content_encryption.py
[info] 280-280: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"report": report})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
frontends/aiq_api/tests/test_content_encryption_routes.py
[info] 321-321: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"report": "secret"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
frontends/aiq_api/src/aiq_api/routes/jobs.py
[info] 1224-1224: use jsonify instead of json.dumps for JSON output
Context: json.dumps({'error': 'Content encryption is unavailable'})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1227-1227: use jsonify instead of json.dumps for JSON output
Context: json.dumps({'error': 'Job event data is invalid'})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
frontends/aiq_api/src/aiq_api/jobs/event_store.py
[info] 405-405: use jsonify instead of json.dumps for JSON output
Context: json.dumps(stored_event)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 465-465: use jsonify instead of json.dumps for JSON output
Context: json.dumps(stored_event)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
frontends/aiq_api/src/aiq_api/jobs/crypto.py
[info] 864-864: use jsonify instead of json.dumps for JSON output
Context: json.dumps(value)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 899-899: use jsonify instead of json.dumps for JSON output
Context: json.dumps(envelope, separators=(",", ":"), sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 930-930: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1087-1087: use secrets package over random package
Context: random.uniform(0, _VAULT_RETRY_JITTER_SECONDS)
Note: [CWE-330] Use of Insufficiently Random Values.
(avoid-random-python)
🪛 Trivy (0.69.3)
deploy/Dockerfile
[error] 79-82: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y curl ca-certificates && rm -rf /var/lib/apt/lists/*'
Rule: DS-0029
(IaC/Dockerfile)
🔇 Additional comments (10)
deploy/Dockerfile (1)
82-82: LGTM!frontends/aiq_api/pyproject.toml (1)
50-51: LGTM!docs/source/deployment/content-encryption.md (1)
1-205: LGTM!docs/source/deployment/index.md (1)
34-35: LGTM!docs/source/index.md (1)
100-100: LGTM!frontends/aiq_api/src/aiq_api/jobs/crypto.py (1)
16-98: LGTM!Also applies to: 150-897, 918-981, 1007-1094
frontends/aiq_api/src/aiq_api/jobs/event_store.py (1)
25-32: LGTM!Also applies to: 107-169, 405-406, 461-466, 539-562, 591-593, 619-621, 652-673
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
316-316: LGTM!Also applies to: 328-341, 484-484, 542-560
frontends/aiq_api/src/aiq_api/jobs/submit.py (1)
120-123: LGTM!Also applies to: 142-143, 162-163, 208-210
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
316-327: LGTM!Also applies to: 427-447, 660-675, 699-715, 1161-1199, 1215-1228, 1252-1424, 1454-1575
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
316-327: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove startup encryption validation behind the async-route availability gate.
Line 326 now makes route registration depend on Vault/content-encryption readiness before the
dask_available/job_storecheck runs. That means a transient encryption startup failure can prevent/v1/data_sourcesand/v1/jobs/async/agentsfrom registering at all, even though those endpoints do not consume encrypted job content. Scope this validation to the async-job surface instead of the whole router setup.Suggested change
- await validate_content_encryption_startup_async() - if not get_all_sources(): logger.warning( "No data sources registered. Add a 'data_sources' function with " @@ if not dask_available or not job_store: logger.warning( "Dask not available - async job submission routes require NAT_DASK_SCHEDULER_ADDRESS" " and NAT_JOB_STORE_DB_URL" ) return + + await validate_content_encryption_startup_async()As per path instructions, treat these API/job-runner route changes as externally visible contracts and preserve the async-job lifecycle boundaries.
Also applies to: 373-378
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/aiq_api/src/aiq_api/routes/jobs.py` around lines 316 - 327, Move the validate_content_encryption_startup_async() call out of the top-level jobs router setup so it only runs when wiring the async-job endpoints, not before the dask_available/job_store gate in jobs.py. Keep the startup encryption check scoped to the async job route registration path used by submit_authorized_job and the related async job handlers, so transient Vault/content-encryption issues cannot block unrelated routes like /v1/data_sources from registering.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontends/aiq_api/src/aiq_api/routes/jobs.py`:
- Around line 458-462: The documented 500 response in the jobs route does not
match the handler’s actual behavior. Update the OpenAPI `responses` entry in the
job submission endpoint so `500` describes only the real failure cases handled
by the route, namely invalid content encryption configuration and
authorization-metadata persistence failures in the relevant job submission flow.
Keep the response text aligned with the logic in the route handler and any
related tests in `test_content_encryption_routes.py`.
---
Outside diff comments:
In `@frontends/aiq_api/src/aiq_api/routes/jobs.py`:
- Around line 316-327: Move the validate_content_encryption_startup_async() call
out of the top-level jobs router setup so it only runs when wiring the async-job
endpoints, not before the dask_available/job_store gate in jobs.py. Keep the
startup encryption check scoped to the async job route registration path used by
submit_authorized_job and the related async job handlers, so transient
Vault/content-encryption issues cannot block unrelated routes like
/v1/data_sources from registering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b8a7f5ff-955f-4f17-897c-d11b02f2f755
📒 Files selected for processing (5)
frontends/aiq_api/src/aiq_api/jobs/crypto.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
tests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
tests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}: Treat API, auth, and job-runner changes as externally visible contracts. Check authorization boundaries,
request tracing, async job lifecycle, websocket reconnect behavior, error responses, and cross-user data isolation.
Require tests for route behavior, access decisions, and job state transitions when those surfaces change.
Files:
frontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
🧠 Learnings (1)
📚 Learning: 2026-06-14T17:49:00.640Z
Learnt from: torkian
Repo: NVIDIA-AI-Blueprints/aiq PR: 273
File: frontends/aiq_api/tests/test_sse_reconnect_cursor.py:384-401
Timestamp: 2026-06-14T17:49:00.640Z
Learning: When using `unittest.mock.patch` for code that imports dependencies inside functions/generators (e.g., inside `aiq_api.routes.jobs`), don’t patch via an attribute that doesn’t exist on the consuming module. If the generator does `from ..jobs.event_store import EventStore` inside the generator body, then `aiq_api.routes.jobs` will not have an `EventStore` attribute; patch the source class/method in its defining module instead (e.g., `aiq_api.jobs.event_store.EventStore.get_events_async`). Patching `aiq_api.routes.jobs.EventStore...` would raise `AttributeError` because that symbol is not present at module scope.
Applied to files:
frontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.py
🔇 Additional comments (5)
frontends/aiq_api/src/aiq_api/jobs/crypto.py (2)
905-916:decode_envelope()still misses the non-ASCII failure path.Non-ASCII payloads like the new regression case in
frontends/aiq_api/tests/test_content_encryption.pyLines 263-265 still fail in_b64url_decode()before this handler, so they bypass theContentEncryptionInvalidDatamapping unlessUnicodeEncodeErroris caught here too.
30-30: LGTM!Also applies to: 106-143, 991-1017
frontends/aiq_api/tests/test_content_encryption.py (1)
117-227: LGTM!Also applies to: 263-265
frontends/aiq_api/tests/test_content_encryption_routes.py (1)
101-103: LGTM!Also applies to: 247-258
tests/aiq_agent/jobs/test_runner.py (1)
600-678: LGTM!
Redact credential-bearing configuration fields and fingerprint manager signatures without retaining raw secrets. Reject non-finite timing values, clarify async submission response contracts, and isolate the encryption failure database test. Add regression coverage for credential handling, manager recreation, malformed envelopes, route submission mocks, and OpenAPI responses. Signed-off-by: Tanner Leach <tleach@nvidia.com>
tanleach
force-pushed
the
feat/report_encryption
branch
from
cf0b282 to
40e854b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
326-326: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not let encryption startup validation prevent health and public route registration.
Line 326 can abort registration before
/health,/v1/data_sources, and agent listing are available, so a missing/invalid encryption secret crashes the surface that should report degradation. CatchContentEncryptionConfigError/ContentEncryptionUnavailablehere and keep submit/read paths gated by their existing checks.Proposed fix
- await validate_content_encryption_startup_async() + try: + await validate_content_encryption_startup_async() + except (ContentEncryptionConfigError, ContentEncryptionUnavailable) as exc: + logger.warning( + "Content encryption startup validation failed; routes will report readiness failures: %s", + exc.__class__.__name__, + )As per coding guidelines, “Missing-secret paths must degrade gracefully (stub/skip), not crash or leak.” As per path instructions, treat API route behavior as an externally visible contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/aiq_api/src/aiq_api/routes/jobs.py` at line 326, Move the startup encryption validation in the jobs route setup so it does not block registration of public/health endpoints. In jobs.py around the route registration flow, catch ContentEncryptionConfigError and ContentEncryptionUnavailable around validate_content_encryption_startup_async() and allow /health, /v1/data_sources, and agent listing routes to register even when encryption is misconfigured. Keep the submit/read handlers protected by their existing per-route checks so only encryption-dependent paths are gated.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontends/aiq_api/src/aiq_api/routes/jobs.py`:
- Line 326: Move the startup encryption validation in the jobs route setup so it
does not block registration of public/health endpoints. In jobs.py around the
route registration flow, catch ContentEncryptionConfigError and
ContentEncryptionUnavailable around validate_content_encryption_startup_async()
and allow /health, /v1/data_sources, and agent listing routes to register even
when encryption is misconfigured. Keep the submit/read handlers protected by
their existing per-route checks so only encryption-dependent paths are gated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 65983828-0ce4-42b7-bbaa-c8886623a9fd
📒 Files selected for processing (5)
frontends/aiq_api/src/aiq_api/jobs/crypto.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Run Harbor skill eval
- GitHub Check: Lint and Hooks
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
frontends/aiq_api/tests/test_content_encryption.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
frontends/aiq_api/tests/test_content_encryption.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_content_encryption_routes.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
frontends/aiq_api/tests/test_content_encryption.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}: Treat API, auth, and job-runner changes as externally visible contracts. Check authorization boundaries,
request tracing, async job lifecycle, websocket reconnect behavior, error responses, and cross-user data isolation.
Require tests for route behavior, access decisions, and job state transitions when those surfaces change.
Files:
frontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
🧠 Learnings (1)
📚 Learning: 2026-06-14T17:49:00.640Z
Learnt from: torkian
Repo: NVIDIA-AI-Blueprints/aiq PR: 273
File: frontends/aiq_api/tests/test_sse_reconnect_cursor.py:384-401
Timestamp: 2026-06-14T17:49:00.640Z
Learning: When using `unittest.mock.patch` for code that imports dependencies inside functions/generators (e.g., inside `aiq_api.routes.jobs`), don’t patch via an attribute that doesn’t exist on the consuming module. If the generator does `from ..jobs.event_store import EventStore` inside the generator body, then `aiq_api.routes.jobs` will not have an `EventStore` attribute; patch the source class/method in its defining module instead (e.g., `aiq_api.jobs.event_store.EventStore.get_events_async`). Patching `aiq_api.routes.jobs.EventStore...` would raise `AttributeError` because that symbol is not present at module scope.
Applied to files:
frontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.py
🔇 Additional comments (8)
frontends/aiq_api/src/aiq_api/routes/jobs.py (2)
461-461: Previously flagged: narrow the documented500response.Line 461 still says “job submission failed”, but this handler maps
500to invalid encryption config or async-job authorization metadata persistence failure. This duplicates the existing review thread on this line.
316-325: LGTM!Also applies to: 427-447, 478-534, 661-718
frontends/aiq_api/src/aiq_api/jobs/crypto.py (2)
30-30: LGTM!Also applies to: 984-1005
101-148: LGTM!Also applies to: 1008-1019
frontends/aiq_api/tests/test_content_encryption_routes.py (2)
101-103: 🎯 Functional CorrectnessVerify
_build_jobs_apppatches the submit symbol resolved byregister_job_routes.
submitted_jobis patched onaiq_api.jobs.submit.submit_agent_job, but these tests only intercept the route ifregister_job_routesimports that symbol after the patch. If the handler is bound to a module-local alias instead, the submit success/503 cases will not exercise the intended branch. Based on learnings, patch the symbol where the consuming module resolves it.#!/bin/bash set -euo pipefail ast-grep outline frontends/aiq_api/src/aiq_api/routes/jobs.py --view expanded | sed -n '1,220p' rg -n -C3 'submit_authorized_job|submit_agent_job' \ frontends/aiq_api/src/aiq_api/routes/jobs.py \ frontends/aiq_api/src/aiq_api/jobs/submit.pySource: Learnings
261-270: LGTM!frontends/aiq_api/tests/test_content_encryption.py (1)
117-227: LGTM!Also applies to: 263-265
tests/aiq_agent/jobs/test_runner.py (1)
600-600: LGTM!Also applies to: 639-639, 666-666
Document the submit endpoint's actual HTTP 500 failure cases and update the OpenAPI regression assertion to match. Signed-off-by: Tanner Leach <tleach@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
665-680: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the new state/report encryption error responses.
These handlers now return
503and500, but the decorators still advertise only404, so the OpenAPI contract omits externally visible failure modes.Proposed fix
summary="Get job artifacts", description="Get tool calls, outputs, and sources collected during job execution.", - responses={404: {"description": "Job not found"}}, + responses={ + 404: {"description": "Job not found"}, + 500: {"description": "Job state data is invalid"}, + 503: {"description": "Content encryption is unavailable"}, + }, @@ summary="Get final report", description="Get the final research report from a completed job.", - responses={404: {"description": "Job not found"}}, + responses={ + 404: {"description": "Job not found"}, + 500: {"description": "Final report data is invalid"}, + 503: {"description": "Content encryption is unavailable"}, + },As per path instructions, treat API error responses as externally visible contracts.
Also applies to: 704-718
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/aiq_api/src/aiq_api/routes/jobs.py` around lines 665 - 680, The job artifact handlers in the jobs route now return additional HTTP failures, but the OpenAPI responses still only advertise 404. Update the response documentation on the affected endpoint decorators and any shared response definitions used by the artifact/state reporting paths (including the helper around the job artifact fetch flow) so the contract explicitly includes the new 503 from ContentEncryptionUnavailable and 500 from ContentEncryptionInvalidData, alongside the existing 404.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontends/aiq_api/src/aiq_api/routes/jobs.py`:
- Around line 665-680: The job artifact handlers in the jobs route now return
additional HTTP failures, but the OpenAPI responses still only advertise 404.
Update the response documentation on the affected endpoint decorators and any
shared response definitions used by the artifact/state reporting paths
(including the helper around the job artifact fetch flow) so the contract
explicitly includes the new 503 from ContentEncryptionUnavailable and 500 from
ContentEncryptionInvalidData, alongside the existing 404.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c2dd4b8d-69b5-4a42-9ee8-084ad5a065be
📒 Files selected for processing (2)
frontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/tests/test_content_encryption_routes.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Run Harbor skill eval
- GitHub Check: Lint and Hooks
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
frontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/src/aiq_api/routes/jobs.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
frontends/aiq_api/tests/test_content_encryption_routes.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
frontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/src/aiq_api/routes/jobs.py
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}: Treat API, auth, and job-runner changes as externally visible contracts. Check authorization boundaries,
request tracing, async job lifecycle, websocket reconnect behavior, error responses, and cross-user data isolation.
Require tests for route behavior, access decisions, and job state transitions when those surfaces change.
Files:
frontends/aiq_api/src/aiq_api/routes/jobs.py
🧠 Learnings (1)
📚 Learning: 2026-06-14T17:49:00.640Z
Learnt from: torkian
Repo: NVIDIA-AI-Blueprints/aiq PR: 273
File: frontends/aiq_api/tests/test_sse_reconnect_cursor.py:384-401
Timestamp: 2026-06-14T17:49:00.640Z
Learning: When using `unittest.mock.patch` for code that imports dependencies inside functions/generators (e.g., inside `aiq_api.routes.jobs`), don’t patch via an attribute that doesn’t exist on the consuming module. If the generator does `from ..jobs.event_store import EventStore` inside the generator body, then `aiq_api.routes.jobs` will not have an `EventStore` attribute; patch the source class/method in its defining module instead (e.g., `aiq_api.jobs.event_store.EventStore.get_events_async`). Patching `aiq_api.routes.jobs.EventStore...` would raise `AttributeError` because that symbol is not present at module scope.
Applied to files:
frontends/aiq_api/tests/test_content_encryption_routes.py
🔇 Additional comments (2)
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
316-327: LGTM!Also applies to: 427-447, 461-466, 482-496, 525-538, 1414-1429
frontends/aiq_api/tests/test_content_encryption_routes.py (1)
142-195: LGTM!Also applies to: 198-244, 269-271, 448-478
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
Two blocking encryption-boundary issues found in the current PR head.
Propagate a non-secret encryption policy identity with async job submissions and fail workers before RUNNING when their local mode or key identity differs. Document the boundary and cover static-key, Vault, missing-policy, and API-to-worker mismatch behavior. Signed-off-by: Tanner Leach <tleach@nvidia.com>
Propagate event persistence failures when content encryption is enabled and retain background batch flush errors for the runner to observe. Keep off-mode event writes best effort and add timer and job-state regression coverage. Signed-off-by: Tanner Leach <tleach@nvidia.com>
Replace NAT's generic health route with AI-Q readiness so runtime dispatch and OpenAPI expose database and encryption state. Preserve the healthy success response and cover assembled worker behavior for unavailable Vault, off mode, and static-key mode. Signed-off-by: Tanner Leach <tleach@nvidia.com>
Resolve async job runner, submission, route, and test conflicts with the latest develop branch. Preserve encryption policy enforcement across the expanded worker arguments, encrypt report interaction metadata, and decrypt parent reports for report follow-up jobs. Signed-off-by: Tanner Leach <tleach@nvidia.com>
Add a process-only /live endpoint while retaining dependency checks on /health, and wire Helm probes to the correct paths. Map report-edit submission encryption outages to 503 and add regression coverage for health routing, auth exemptions, chart probes, and errors. Signed-off-by: Tanner Leach <tleach@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/integration/rest-api.md`:
- Around line 481-487: The /health example is incomplete and should match the
actual readiness payload. Update the health response sample to include the full
set of encryption health fields produced by the readiness endpoint, not just
encryption.mode and encryption.ready, so the documented contract reflects the
current behavior of the /health response.
In `@frontends/aiq_api/tests/test_report_context.py`:
- Around line 298-341: Wrap the cache setup and assertions in
test_resolve_authorized_report_context_caches_job_store with a try/finally so
_JOB_STORE_CACHE.clear() always runs even if an assertion or await raises; this
prevents the cached _FakeJobStore for the configured scheduler_address and
db_url from leaking into other tests. Use the existing
report_context._JOB_STORE_CACHE cleanup pattern from the other tests in
test_report_context.py and keep the teardown in the finally block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7635ea2d-1de2-428e-8698-fc659866436a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
deploy/helm/README.mddeploy/helm/deployment-k8s/README.mddeploy/helm/deployment-k8s/values.yamldocs/source/deployment/content-encryption.mddocs/source/deployment/index.mddocs/source/deployment/kubernetes.mddocs/source/deployment/production.mddocs/source/index.mddocs/source/integration/rest-api.mdfrontends/aiq_api/src/aiq_api/auth/middleware.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pyfrontends/aiq_api/src/aiq_api/jobs/report_context.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/tests/test_auth.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/tests/test_report_context.pyfrontends/aiq_api/tests/test_report_edit.pyfrontends/aiq_api/tests/test_submit_owner_user_id.pytests/aiq_agent/jobs/test_runner.pytests/deploy/test_helm_deployment_k8s.py
💤 Files with no reviewable changes (2)
- tests/deploy/test_helm_deployment_k8s.py
- tests/aiq_agent/jobs/test_runner.py
📜 Review details
⚠️ CI failures not shown inline (5)
GitHub Actions: Skills Eval / 0_Run Harbor skill eval.txt: fix(api): separate liveness from dependency readiness
Conclusion: failure
##[group]Run if [ ! -r "$RUNNER_ENV_FILE" ]; then
�[36;1mif [ ! -r "$RUNNER_ENV_FILE" ]; then�[0m
�[36;1m echo "::error::Runner credential file missing or unreadable at $RUNNER_ENV_FILE"�[0m
GitHub Actions: Skills Eval / Run Harbor skill eval: fix(api): separate liveness from dependency readiness
Conclusion: failure
##[group]Run # Poll up to 5 minutes. AI-Q cold start on the runner is typically
�[36;1m# Poll up to 5 minutes. AI-Q cold start on the runner is typically�[0m
�[36;1m# <2 min; 5 min gives margin for the postgres init + first build.�[0m
�[36;1mfor i in $(seq 1 60); do�[0m
�[36;1m if curl -sf http://localhost:8000/health >/dev/null 2>&1; then�[0m
�[36;1m echo "AI-Q healthy after ${i} x 5s"�[0m
�[36;1m curl -s http://localhost:8000/health | head -c 400; echo�[0m
�[36;1m exit 0�[0m
�[36;1m fi�[0m
�[36;1m sleep 5�[0m
�[36;1mdone�[0m
�[36;1mecho "::error::AI-Q did not report healthy within 5 minutes"�[0m
GitHub Actions: Skills Eval / Run Harbor skill eval: fix(api): separate liveness from dependency readiness
Conclusion: failure
##[group]Run if [ ! -r "$RUNNER_ENV_FILE" ]; then
�[36;1mif [ ! -r "$RUNNER_ENV_FILE" ]; then�[0m
�[36;1m echo "::error::Runner credential file missing or unreadable at $RUNNER_ENV_FILE"�[0m
GitHub Actions: AIQ CI / Script Validation: fix(api): separate liveness from dependency readiness
Conclusion: failure
##[group]Run . .venv/bin/activate
�[36;1m. .venv/bin/activate�[0m
�[36;1mchmod +x ci/scripts/test_scripts.sh�[0m
�[36;1mci/scripts/test_scripts.sh --skip-setup�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.13.14/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.13.14/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.13.14/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.13.14/x64/lib
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
================================================
AI-Q Blueprint - Script Tests
================================================
Repository: /home/runner/work/aiq/aiq
Scripts: /home/runner/work/aiq/aiq/scripts
============================================
Testing Bash Syntax
============================================
�[0;32m✅ PASS�[0m: dev.sh - valid bash syntax
�[0;32m✅ PASS�[0m: setup.sh - valid bash syntax
�[0;32m✅ PASS�[0m: setup_openshell.sh - valid bash syntax
�[0;32m✅ PASS�[0m: start_as_skill.sh - valid bash syntax
�[0;32m✅ PASS�[0m: start_cli.sh - valid bash syntax
�[0;32m✅ PASS�[0m: start_e2e.sh - valid bash syntax
�[0;32m✅ PASS�[0m: start_server_in_debug_mode.sh - valid bash syntax
�[1;33m⏭️ SKIP�[0m: setup.sh - skipped (--skip-setup flag)
============================================
Testing --help Flags
============================================
�[0;32m✅ PASS�[0m: start_cli.sh --help
�[0;32m✅ PASS�[0m: start_server_in_debug_mode.sh --help
============================================
Testing Virtual Environment Checks
============================================
�[0;32m✅ PASS�[0m: start_cli.sh - venv check works
�[0;32m✅ PASS�[0m: start_server_in_debug_mode.sh - venv check works
============================================
Testing Pytest Integration
=====================...
GitHub Actions: AIQ CI / 2_Script Validation.txt: fix(api): separate liveness from dependency readiness
Conclusion: failure
Current runner version: '2.335.1'
##[group]Runner Image Provisioner
Hosted Compute Agent
Version: 20260624.560
Commit: 925d229a51159bc391ae97e54a2dd1fe20af789d
Build Date:
Worker ID: {b37f07bb-1afe-4944-82c6-7c3e99cf18c4}
Azure Region: eastus
##[endgroup]
##[group]Operating System
Ubuntu
24.04.4
LTS
##[endgroup]
##[group]Runner Image
Image: ubuntu-24.04
Version: 20260628.225.1
Included Software: https://github.com/actions/runner-images/blob/ubuntu24/20260628.225/images/ubuntu/Ubuntu2404-Readme.md
Image Release: https://github.com/actions/runner-images/releases/tag/ubuntu24%2F20260628.225
##[endgroup]
##[group]GITHUB_TOKEN Permissions
Contents: read
Metadata: read
##[endgroup]
Secret source: Actions
Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v4' (SHA:34e114876b0b11c390a56381ad16ebd13914f8d5)
Download action repository 'actions/setup-python@v5' (SHA:a26af69be951a213d495a4c3e4e4022e16d87065)
Download action repository 'astral-sh/setup-uv@v4' (SHA:38f3f104447c67c051c4a08e39b64a148898af3a)
Complete job name: Script Validation
Node 20 is being deprecated. This workflow is running with Node 24 by default. If you need to temporarily use Node 20, you can set the ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION=true environment variable. For more information see: https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/
##[group]Run actions/checkout@v4
with:
repository: NVIDIA-AI-Blueprints/aiq
***REDACTED***
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
fetch-depth: 1
fetch-tags: false
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
##[endgroup]
Syncing repository: NVIDIA-AI-Blueprints/aiq
##[group]Getting Git version info
Working directory is '/home/runner/work/aiq/aiq'
[command]/usr/bin/git version
git v...
🧰 Additional context used
📓 Path-based instructions (8)
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update the docs under docs/source/ when behavior, configuration, or workflows change
Files:
docs/source/deployment/index.mddocs/source/deployment/kubernetes.mddocs/source/index.mddocs/source/deployment/production.mddocs/source/integration/rest-api.mddocs/source/deployment/content-encryption.md
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
docs/source/deployment/index.mddeploy/helm/deployment-k8s/values.yamldocs/source/deployment/kubernetes.mddocs/source/index.mdfrontends/aiq_api/src/aiq_api/auth/middleware.pydeploy/helm/README.mddeploy/helm/deployment-k8s/README.mddocs/source/deployment/production.mdfrontends/aiq_api/tests/test_submit_owner_user_id.pyfrontends/aiq_api/tests/test_report_edit.pyfrontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/jobs/report_context.pydocs/source/integration/rest-api.mdfrontends/aiq_api/tests/test_auth.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pydocs/source/deployment/content-encryption.mdfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/tests/test_report_context.py
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/source/deployment/index.mddocs/source/deployment/kubernetes.mddocs/source/index.mddocs/source/deployment/production.mddocs/source/integration/rest-api.mddocs/source/deployment/content-encryption.md
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
deploy/helm/deployment-k8s/values.yamldeploy/helm/README.mddeploy/helm/deployment-k8s/README.md
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
frontends/aiq_api/src/aiq_api/auth/middleware.pyfrontends/aiq_api/tests/test_submit_owner_user_id.pyfrontends/aiq_api/tests/test_report_edit.pyfrontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/jobs/report_context.pyfrontends/aiq_api/tests/test_auth.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/tests/test_report_context.py
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}: Treat API, auth, and job-runner changes as externally visible contracts. Check authorization boundaries,
request tracing, async job lifecycle, websocket reconnect behavior, error responses, and cross-user data isolation.
Require tests for route behavior, access decisions, and job state transitions when those surfaces change.
Files:
frontends/aiq_api/src/aiq_api/auth/middleware.pyfrontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/jobs/report_context.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.py
{src/aiq_agent/auth/**,frontends/aiq_api/src/aiq_api/auth/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/auth/**,frontends/aiq_api/src/aiq_api/auth/**}: Review authentication changes for issuer/audience validation, token parsing, error hygiene, logging safety,
and compatibility with local and deployed modes. Do not accept changes that expose tokens, weaken validation,
or blur trusted server-side identity with client-supplied fields.
Files:
frontends/aiq_api/src/aiq_api/auth/middleware.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
frontends/aiq_api/tests/test_submit_owner_user_id.pyfrontends/aiq_api/tests/test_report_edit.pyfrontends/aiq_api/tests/test_auth.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/tests/test_report_context.py
🧠 Learnings (1)
📚 Learning: 2026-06-14T17:49:00.640Z
Learnt from: torkian
Repo: NVIDIA-AI-Blueprints/aiq PR: 273
File: frontends/aiq_api/tests/test_sse_reconnect_cursor.py:384-401
Timestamp: 2026-06-14T17:49:00.640Z
Learning: When using `unittest.mock.patch` for code that imports dependencies inside functions/generators (e.g., inside `aiq_api.routes.jobs`), don’t patch via an attribute that doesn’t exist on the consuming module. If the generator does `from ..jobs.event_store import EventStore` inside the generator body, then `aiq_api.routes.jobs` will not have an `EventStore` attribute; patch the source class/method in its defining module instead (e.g., `aiq_api.jobs.event_store.EventStore.get_events_async`). Patching `aiq_api.routes.jobs.EventStore...` would raise `AttributeError` because that symbol is not present at module scope.
Applied to files:
frontends/aiq_api/tests/test_submit_owner_user_id.pyfrontends/aiq_api/tests/test_report_edit.pyfrontends/aiq_api/tests/test_auth.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/tests/test_report_context.py
🪛 ast-grep (0.44.1)
frontends/aiq_api/tests/test_report_edit.py
[info] 119-119: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"report": parent_report})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
frontends/aiq_api/src/aiq_api/jobs/report_context.py
[warning] 91-91: Do not make http calls without encryption
Context: "http://"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
frontends/aiq_api/tests/test_content_encryption_routes.py
[info] 521-528: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"report": "secret",
"parent_job_id": "parent-job",
"interaction_action": "edit",
"result_kind": "report",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
frontends/aiq_api/tests/test_report_context.py
[info] 203-203: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"report": report})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (26)
frontends/aiq_api/src/aiq_api/jobs/report_context.py (2)
91-92: The ast-greprequests-httphit here is a false positive: this is a URL-scheme predicate over report markdown, not an outbound request. No action needed.
184-221: LGTM!frontends/aiq_api/src/aiq_api/jobs/crypto.py (1)
105-194: LGTM!Also applies to: 846-857
frontends/aiq_api/src/aiq_api/jobs/event_store.py (1)
116-169: LGTM!Also applies to: 445-446, 504-505, 544-567, 818-874
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
454-457: LGTM!Also applies to: 546-566, 808-829, 846-911, 1219-1231
frontends/aiq_api/src/aiq_api/jobs/submit.py (1)
155-158: LGTM!Also applies to: 252-254, 321-324
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
68-80: LGTM!Also applies to: 523-607, 689-701, 824-907, 1128-1157
frontends/aiq_api/src/aiq_api/auth/middleware.py (1)
185-201: LGTM!frontends/aiq_api/tests/test_auth.py (1)
896-914: LGTM!Also applies to: 945-957
docs/source/deployment/content-encryption.md (1)
177-216: LGTM!docs/source/deployment/index.md (1)
36-37: LGTM!docs/source/index.md (1)
101-101: LGTM!docs/source/deployment/kubernetes.md (1)
157-164: LGTM!docs/source/deployment/production.md (1)
118-129: LGTM!docs/source/integration/rest-api.md (1)
34-39: LGTM!Also applies to: 82-104, 106-149, 455-471
deploy/helm/README.md (1)
134-166: LGTM!Also applies to: 192-199, 243-283
deploy/helm/deployment-k8s/README.md (1)
71-78: LGTM!deploy/helm/deployment-k8s/values.yaml (1)
52-52: LGTM!frontends/aiq_api/tests/test_content_encryption_routes.py (2)
83-140: 🎯 Functional CorrectnessConfirm the submit-route patch target was fixed.
A prior automated review on this exact file flagged that
register_job_routesdispatches submission viasubmit_authorized_job(resolved inaiq_api.routes.jobs), so patchingaiq_api.jobs.submit.submit_agent_jobnever intercepts the call under test — making the 503 submit assertions pass without proving submission was actually skipped. The summary for this range still describes_build_jobs_appas "optionally patch[ing]submit_agent_job", so it's unclear if the fix landed.🔍 Verification script
#!/bin/bash # Confirm what symbol the submit route actually calls, and what this test patches. rg -n 'submit_authorized_job' frontends/aiq_api/src/aiq_api/routes/jobs.py -B2 -A5 rg -n 'def submit_authorized_job|submit_authorized_job\s*=' frontends/aiq_api/src/aiq_api -A5 rg -n 'submit_agent_job|submit_authorized_job' frontends/aiq_api/tests/test_content_encryption_routes.pySource: Learnings
204-664: LGTM!frontends/aiq_api/tests/test_content_encryption.py (1)
160-205: LGTM!frontends/aiq_api/tests/test_event_store_content_encryption.py (1)
148-217: LGTM!frontends/aiq_api/tests/test_report_context.py (1)
10-295: LGTM!frontends/aiq_api/tests/test_report_edit.py (2)
325-350: LGTM! This GET-report test doesn't depend on the submission mock and is unaffected by the concern above.
18-64: 🎯 Functional CorrectnessNo issue:
submit_agent_jobis patched beforeregister_job_routes()imports it, sosubmit_authorized_jobbinds to the mock and these tests exercise the intended submission path.> Likely an incorrect or invalid review comment.frontends/aiq_api/tests/test_submit_owner_user_id.py (1)
20-82: LGTM!
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/integration/rest-api.md`:
- Around line 481-487: The /health example is incomplete and should match the
actual readiness payload. Update the health response sample to include the full
set of encryption health fields produced by the readiness endpoint, not just
encryption.mode and encryption.ready, so the documented contract reflects the
current behavior of the /health response.
In `@frontends/aiq_api/tests/test_report_context.py`:
- Around line 298-341: Wrap the cache setup and assertions in
test_resolve_authorized_report_context_caches_job_store with a try/finally so
_JOB_STORE_CACHE.clear() always runs even if an assertion or await raises; this
prevents the cached _FakeJobStore for the configured scheduler_address and
db_url from leaking into other tests. Use the existing
report_context._JOB_STORE_CACHE cleanup pattern from the other tests in
test_report_context.py and keep the teardown in the finally block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7635ea2d-1de2-428e-8698-fc659866436a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
deploy/helm/README.mddeploy/helm/deployment-k8s/README.mddeploy/helm/deployment-k8s/values.yamldocs/source/deployment/content-encryption.mddocs/source/deployment/index.mddocs/source/deployment/kubernetes.mddocs/source/deployment/production.mddocs/source/index.mddocs/source/integration/rest-api.mdfrontends/aiq_api/src/aiq_api/auth/middleware.pyfrontends/aiq_api/src/aiq_api/jobs/crypto.pyfrontends/aiq_api/src/aiq_api/jobs/event_store.pyfrontends/aiq_api/src/aiq_api/jobs/report_context.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/src/aiq_api/jobs/submit.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/tests/test_auth.pyfrontends/aiq_api/tests/test_content_encryption.pyfrontends/aiq_api/tests/test_content_encryption_routes.pyfrontends/aiq_api/tests/test_event_store_content_encryption.pyfrontends/aiq_api/tests/test_report_context.pyfrontends/aiq_api/tests/test_report_edit.pyfrontends/aiq_api/tests/test_submit_owner_user_id.pytests/aiq_agent/jobs/test_runner.pytests/deploy/test_helm_deployment_k8s.py
💤 Files with no reviewable changes (2)
- tests/deploy/test_helm_deployment_k8s.py
- tests/aiq_agent/jobs/test_runner.py
📜 Review details
🔇 Additional comments (26)
frontends/aiq_api/src/aiq_api/jobs/report_context.py (2)
91-92: The ast-greprequests-httphit here is a false positive: this is a URL-scheme predicate over report markdown, not an outbound request. No action needed.
184-221: LGTM!frontends/aiq_api/src/aiq_api/jobs/crypto.py (1)
105-194: LGTM!Also applies to: 846-857
frontends/aiq_api/src/aiq_api/jobs/event_store.py (1)
116-169: LGTM!Also applies to: 445-446, 504-505, 544-567, 818-874
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
454-457: LGTM!Also applies to: 546-566, 808-829, 846-911, 1219-1231
frontends/aiq_api/src/aiq_api/jobs/submit.py (1)
155-158: LGTM!Also applies to: 252-254, 321-324
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
68-80: LGTM!Also applies to: 523-607, 689-701, 824-907, 1128-1157
frontends/aiq_api/src/aiq_api/auth/middleware.py (1)
185-201: LGTM!frontends/aiq_api/tests/test_auth.py (1)
896-914: LGTM!Also applies to: 945-957
docs/source/deployment/content-encryption.md (1)
177-216: LGTM!docs/source/deployment/index.md (1)
36-37: LGTM!docs/source/index.md (1)
101-101: LGTM!docs/source/deployment/kubernetes.md (1)
157-164: LGTM!docs/source/deployment/production.md (1)
118-129: LGTM!docs/source/integration/rest-api.md (1)
34-39: LGTM!Also applies to: 82-104, 106-149, 455-471
deploy/helm/README.md (1)
134-166: LGTM!Also applies to: 192-199, 243-283
deploy/helm/deployment-k8s/README.md (1)
71-78: LGTM!deploy/helm/deployment-k8s/values.yaml (1)
52-52: LGTM!frontends/aiq_api/tests/test_content_encryption_routes.py (2)
83-140: 🎯 Functional CorrectnessConfirm the submit-route patch target was fixed.
A prior automated review on this exact file flagged that
register_job_routesdispatches submission viasubmit_authorized_job(resolved inaiq_api.routes.jobs), so patchingaiq_api.jobs.submit.submit_agent_jobnever intercepts the call under test — making the 503 submit assertions pass without proving submission was actually skipped. The summary for this range still describes_build_jobs_appas "optionally patch[ing]submit_agent_job", so it's unclear if the fix landed.🔍 Verification script
#!/bin/bash # Confirm what symbol the submit route actually calls, and what this test patches. rg -n 'submit_authorized_job' frontends/aiq_api/src/aiq_api/routes/jobs.py -B2 -A5 rg -n 'def submit_authorized_job|submit_authorized_job\s*=' frontends/aiq_api/src/aiq_api -A5 rg -n 'submit_agent_job|submit_authorized_job' frontends/aiq_api/tests/test_content_encryption_routes.pySource: Learnings
204-664: LGTM!frontends/aiq_api/tests/test_content_encryption.py (1)
160-205: LGTM!frontends/aiq_api/tests/test_event_store_content_encryption.py (1)
148-217: LGTM!frontends/aiq_api/tests/test_report_context.py (1)
10-295: LGTM!frontends/aiq_api/tests/test_report_edit.py (2)
325-350: LGTM! This GET-report test doesn't depend on the submission mock and is unaffected by the concern above.
18-64: 🎯 Functional CorrectnessNo issue:
submit_agent_jobis patched beforeregister_job_routes()imports it, sosubmit_authorized_jobbinds to the mock and these tests exercise the intended submission path.> Likely an incorrect or invalid review comment.frontends/aiq_api/tests/test_submit_owner_user_id.py (1)
20-82: LGTM!
🛑 Comments failed to post (2)
docs/source/integration/rest-api.md (1)
481-487: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the
/healthexample with the actual payload.The sample is truncated: the readiness response includes additional encryption health fields beyond
mode/ready, so this example teaches the wrong contract.Suggested update
"encryption": { "mode": "off", - "ready": true + "ready": true, + "encrypt_ready": true, + "decrypt_ready": true }As per path instructions, review documentation for command accuracy and stale examples.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."status": "healthy", "dask_available": true, "db": "ok", "encryption": { "mode": "off", "ready": true, "encrypt_ready": true, "decrypt_ready": true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/source/integration/rest-api.md` around lines 481 - 487, The /health example is incomplete and should match the actual readiness payload. Update the health response sample to include the full set of encryption health fields produced by the readiness endpoint, not just encryption.mode and encryption.ready, so the documented contract reflects the current behavior of the /health response.Source: Path instructions
frontends/aiq_api/tests/test_report_context.py (1)
298-341: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrap cache cleanup in
try/finallyto avoid test pollution.If an assertion at Line 340 fails (or an earlier step raises), Line 341's
_JOB_STORE_CACHE.clear()never runs, leaving a stale_FakeJobStorecached for("tcp://localhost:8786", "sqlite:///unused.db")that could leak into unrelated tests. Other tests in this file (e.g. the encrypted-parent-output test) already usetry/finallyfor similar teardown.🧹 Suggested fix
- await report_context.resolve_authorized_report_context("job-1", None) - await report_context.resolve_authorized_report_context("job-1", None) - - assert constructions["n"] == 1 - report_context._JOB_STORE_CACHE.clear() + try: + await report_context.resolve_authorized_report_context("job-1", None) + await report_context.resolve_authorized_report_context("job-1", None) + + assert constructions["n"] == 1 + finally: + report_context._JOB_STORE_CACHE.clear()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@pytest.mark.asyncio` async def test_resolve_authorized_report_context_caches_job_store(monkeypatch): """JobStore is built once per (scheduler_address, db_url), not per request. JobStore eagerly builds a SQLAlchemy AsyncEngine (own connection pool), so per-request construction wastes work and leaks pooled connections. """ from aiq_api.jobs import report_context report_context._JOB_STORE_CACHE.clear() monkeypatch.setenv("NAT_DASK_SCHEDULER_ADDRESS", "tcp://localhost:8786") monkeypatch.setenv("NAT_JOB_STORE_DB_URL", "sqlite:///unused.db") constructions = {"n": 0} class _FakeJobStore: def __init__(self, *, scheduler_address, db_url): constructions["n"] += 1 class _FakeJobStatus: SUCCESS = type("E", (), {"value": "success"}) import nat.front_ends.fastapi.async_jobs.job_store as js monkeypatch.setattr(js, "JobStore", _FakeJobStore) monkeypatch.setattr(js, "JobStatus", _FakeJobStatus) job = type("Job", (), {"status": "success"})() async def _authorize(_store, _db_url, _job_id, _principal): return job async def _resolve(_job, _db_url, _job_id): return report_context.ReportContext(parent_job_id=_job_id, report_markdown="# R", source_summary_markdown="") monkeypatch.setattr(report_context, "authorize_job_access", _authorize) monkeypatch.setattr(report_context, "resolve_report_context", _resolve) try: await report_context.resolve_authorized_report_context("job-1", None) await report_context.resolve_authorized_report_context("job-1", None) assert constructions["n"] == 1 finally: report_context._JOB_STORE_CACHE.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/aiq_api/tests/test_report_context.py` around lines 298 - 341, Wrap the cache setup and assertions in test_resolve_authorized_report_context_caches_job_store with a try/finally so _JOB_STORE_CACHE.clear() always runs even if an assertion or await raises; this prevents the cached _FakeJobStore for the configured scheduler_address and db_url from leaking into other tests. Use the existing report_context._JOB_STORE_CACHE cleanup pattern from the other tests in test_report_context.py and keep the teardown in the finally block.
The /health readiness check only ran the SELECT 1 ping when an async engine happened to already be cached, and reported "db": "no_engine" with HTTP 200 otherwise. On a fresh process the async-engine cache is empty, so /health returned healthy without touching the database. Since Helm now gates readiness on /health, a fresh pod could pass readiness straight through a database outage. Obtain (or create) the engine for the configured db_url via EventStore._get_or_create_async_engine and always run the bounded ping; an absent cached engine is no longer treated as healthy. Add an assembled-app regression test asserting /health returns 503 when the database is unavailable and the async-engine cache is empty, while /live stays 200. Signed-off-by: Tanner Leach <tleach@nvidia.com>
2 participants
Overview
Add application-level envelope encryption at rest for async job final reports
and selected artifact event content. The implementation supports disabled,
static-key, and HashiCorp Vault Transit modes; uses per-job AES-256-GCM data
encryption keys; and fails closed when encrypted report or event data cannot be
authenticated or decrypted.
The hardening in this branch:
concurrent access;
Dask workers and rejects mismatched worker configurations before
RUNNING;than silently losing protected data;
contracts;
/live) from dependency readiness (/health) andconfigures Helm to use the appropriate endpoint for each probe;
exposing exception details; and
operational behavior.
Encryption remains disabled by default. This release intentionally does not
include plaintext migration, backfill, application-managed key rotation,
rewrap, or decrypt-on-rollback tooling. Opt-in operators must follow the
documented forward-only rollout constraints and retain the original encryption
configuration while encrypted jobs exist. Database-writer replay, deletion,
and reordering remain outside the initial confidentiality-at-rest threat model.
Validation
The documentation warnings are pre-existing missing-reference warnings.
The API suite also emits existing dependency deprecation and periodic-cleanup
warnings without test failures.
Manual Docker validation: rebuilt the images from the local Dockerfiles and
successfully ran the application in Docker.
git commit -sor an equivalent sign-off.Where should reviewers start?
Start with
frontends/aiq_api/src/aiq_api/jobs/crypto.pyfor the envelope,readiness, Vault retry, concurrency, and policy-identity model. Then review:
frontends/aiq_api/src/aiq_api/jobs/submit.pyandrunner.pyfor theAPI-to-worker policy boundary;
frontends/aiq_api/src/aiq_api/jobs/event_store.pyfor encrypted persistenceand failure propagation;
frontends/aiq_api/src/aiq_api/routes/jobs.pyfor readiness, report/statedecryption, report-edit submission, and HTTP/SSE error contracts; and
frontends/aiq_api/src/aiq_api/auth/middleware.pyplusdeploy/helm/deployment-k8s/values.yamlfor liveness/readiness exposure.Focused regression coverage is in the content-encryption, report-edit, runner,
authentication, and Helm deployment tests.
Related Issues
Summary by CodeRabbit
New Features
/liveendpoint for basic process health checks.Bug Fixes
Documentation